Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing method to list block. #529

Closed
wants to merge 2 commits into from

Conversation

BE-Webdesign
Copy link
Contributor

@BE-Webdesign BE-Webdesign commented Apr 27, 2017

The list block does not have an on change set for it and is expected to;
causing errors and no updating of the block attribute state. This PR
copies the current implementation from a text block and enters it into
the list block.

@BE-Webdesign BE-Webdesign added the [Type] Bug An existing feature does not function as intended label Apr 27, 2017
@nylen
Copy link
Member

nylen commented Apr 27, 2017

This looks like a good change, but I'm still seeing that this method is not being triggered and any updates are being lost. I'm also unable to focus the list block if I click within a li element - clicking outside one of the item texts works fine. These two things are probably related.

@BE-Webdesign
Copy link
Contributor Author

Yeah good point, it is updating content but the actual items state is not being updated, should the list block keep track of both or should it be unified. Having two sources of truth kinda makes problems like this easy to miss good catch.

In short:

attributes.items is not being kept in sync with attributes.content. Should be easily fixable.

@BE-Webdesign
Copy link
Contributor Author

Just realized there should be no content attribute for the list block oops. Will try and update properly.

The list block does not have an on change set for it and is expected to
causing errors and no updating of the block attribute state.  This PR
copies the current implementation from a text block and enters it into
the list block.
@BE-Webdesign BE-Webdesign force-pushed the fix/list-missing-method-type-error branch from 72a9a0d to 4bf2666 Compare April 27, 2017 23:10
@BE-Webdesign
Copy link
Contributor Author

Okay rebased and updated, so the redux state for a list block will update properly on change.

@youknowriad
Copy link
Contributor

I'm still seeing an error on the console (and the state don't update properly) when I have a single li because this doesn't create an LI.

What bothers me a bit is that the block author needs to be aware of the shape of the underlying data. I'm wondering if it simpler if we change the parsing of items to children( 'ol,ul' )?

@BE-Webdesign
Copy link
Contributor Author

BE-Webdesign commented Apr 28, 2017

I'm still seeing an error on the console (and the state don't update properly) when I have a single li because this doesn't create an LI.

I didn't test for a single list item, so I will fix that. Does the data not come as an array? If not it should be like [ oneItem ], so there is a consistent input output type. I will check into getting this right, good catches.

@youknowriad
Copy link
Contributor

youknowriad commented Apr 28, 2017

Does the data not come as an array?

The data received from children call or from the Editable is any valid React's children prop. It can be an array, an element or a nullable.

@BE-Webdesign
Copy link
Contributor Author

Ah then React made a bad choice then lol.

@BE-Webdesign
Copy link
Contributor Author

Yeah.... this is super buggy lol.

@BE-Webdesign
Copy link
Contributor Author

BE-Webdesign commented Apr 28, 2017

TinyMCE or something is doing some very bizarre things in the list block. The latest update seems to work a bit better as far as the state goes, but sometimes odd things appear in the state from TinyMCE.

Adds in conditional logic to account for when there is a list of
elements, a single element, or a null element.
@androb
Copy link
Contributor

androb commented Apr 28, 2017

@EphoxJames you might want to look at this

@BE-Webdesign
Copy link
Contributor Author

BE-Webdesign commented Apr 28, 2017

I don't know enough about TinyMCE to know what it is doing. Basically sometimes the content will be an empty <br /> with data-mce-bogus being passed forward. From my understanding that is when the data is empty? It is unclear whether this should be passed forward into the Redux state or what the correct line of action is exactly.

@BE-Webdesign
Copy link
Contributor Author

BE-Webdesign commented Apr 28, 2017

Just tested and the <br /> is applied to the block state regardless everytime for any block type when it is made empty. Not sure if that is what we want to have happen. Going to open a separate issue #561 .

@nylen
Copy link
Member

nylen commented Apr 28, 2017

I just opened #567 which looks potentially related. However, the changes in this branch do not fix the issues I noted there - in fact, the new onChange method added in this PR is never called at all for me.

@mtias mtias added the [Feature] Blocks Overall functionality of blocks label May 3, 2017
@BE-Webdesign
Copy link
Contributor Author

Anything that needs to be changed on this?

@nylen
Copy link
Member

nylen commented May 5, 2017

@BE-Webdesign this PR didn't fix the issues noted in the description for me. They've since been fixed, though I'm not sure exactly where.

Separately, #598 achieved most of what we wanted to do here, so I think we can close this one out. Please reopen if I missed something.

@nylen nylen closed this May 5, 2017
@nylen nylen deleted the fix/list-missing-method-type-error branch May 5, 2017 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants